test(ec2): add security related unit tests #5778
Conversation
There was a problem hiding this comment.
Please avoid renaming this file. And the introduction of the "class" abstraction looks unnecessary, or at least it should wait until all feature-branches are merged in the upcoming weeks. Because there are similar changes on a feature branch (but without the class abstraction), so if your necessary changes are small, it will help avoid big noisy merge conflicts in this file.
There was a problem hiding this comment.
I introduced the class to make it easier to spy/stub the methods. I wanted to have tests that assert some of these functions were called. Is there a natural way to do this without the class abstraction?
There was a problem hiding this comment.
Maybe not, but the test added here only stubs findSshPath and the test seems over-specific. Is it important to test the "dry run" behavior? Testing the actual result is usually much stronger coverage (and it looks like that's currently missing for findSshPath?).
Meanwhile, avoiding churn in pathFind.ts will be very helpful for upcoming feature-branch merges.
There was a problem hiding this comment.
I agree that its a little over-specific to the implementation, but AppSec was asking for a test to show we perform some kind of "validation" of the ssh path found and I am unsure how else to show that.
Testing findSshPath without stubs would involve some manipulation of the test environment $PATH variable, which seems risky unless we already have some well-tested utility code to do this for us.
I found another way to stub/spy the method that significantly reduces churn in pathFind.ts. If we move tryRun to another file, and import it as a module, we can stub it in the tests. I think this also makes sense since tryRun doesn't fit in perfectly with all the path finding code.
Still working on fixing the tests on Windows.
| const workspace = await testutil.createTestWorkspaceFolder() | ||
| const fakeSshPath = path.join(workspace.uri.fsPath, `ssh${isWin() ? '.cmd' : ''}`) | ||
|
|
||
| await testutil.withEnvPath(workspace.uri.fsPath, async () => { |
There was a problem hiding this comment.
if we just set $PATH here directly, can afterEach restore it?
the "with" pattern can be useful but it might be confusing if we have different ways of saving/storing state in tests (but maybe there is a good reason for it)
There was a problem hiding this comment.
Yeah, this definitely reduces the amount of code as well. My only concern is that adding more behavior into beforeEach and afterEach can make it hard to isolate the behavior of each test, and sometimes makes it harder to add tests if it doesn't fit with the pattern.
I think this is still the better route since it is inline with the rest of the test code (ex. sinon, clock).
Co-authored-by: Justin M. Keyes <[email protected]>
Co-authored-by: Justin M. Keyes <[email protected]>
Co-authored-by: Justin M. Keyes <[email protected]>
Co-authored-by: Justin M. Keyes <[email protected]>
Co-authored-by: Justin M. Keyes <[email protected]>
Hweinstock
force-pushed
the
securityTests
branch
from
c38c431 to
a28c33e
Compare
Co-authored-by: Justin M. Keyes <[email protected]>
Co-authored-by: Justin M. Keyes <[email protected]>
Co-authored-by: Justin M. Keyes <[email protected]>
| it('modifies the node environment variables (Non-Windows)', function () { | ||
| // PATH returns undefined on Windows. | ||
| if (isWin()) { | ||
| this.skip() |
There was a problem hiding this comment.
PATH should not be undefined on Windows. Even if it did, if I'm reading the test correctly, it looks like it should still pass
3 participants
Problem
We want unit tests to ensure certain security measures are maintained in the code. Properties tested in this PR include:
Solution
For each test we do the following:
getTestWindow()to ensure user's are able to see policies added, and cancel the process.tryRunto ensure it is used before returning the ssh path.As part of (3), we refactor the pathfinding code to be contained in its own class. This makes the code easier to test since we can now use
sinon.spyandsinon.stub. It also allows us to move some of the shared logic into one place.License: I confirm that my contribution is made under the terms of the Apache 2.0 license.